Flink: Support Lookup Join using full in-memory lookup cache - #18144
Guosmilesmile wants to merge 4 commits into
Conversation
|
For snapshot pinning, I was thinking we could resolve the snapshot at job submission time and pass the snapshot ID to every TaskManager, so that all caches use the same snapshot. But there is a problem with this approach. The problem is failover. Flink doesn't re-run the planner when a task restarts, so the pinned snapshot ID would remain unchanged. If that snapshot has expired by then, the cache load would fail with Cannot find snapshot with ID ... after the restart, and retries would keep failing until the job is resubmitted. Periodic refresh doesn't help here, because the failure happens during the initial cache load. We also don't have a way to persist and update the snapshot ID from the connector side, since a lookup join doesn't have checkpointed state. So while pinning the snapshot would ensure consistency across subtasks, it introduces a failover problem. Additionally, dimension tables do not change frequently. For this PR, I therefore use the latest snapshot at the moment and just added logging and a metric to report the snapshot ID being used.If you have a better idea, we'd be happy to take a look. |
6550de0 to
de77ef7
Compare
|
why not use ReadOptions and pass when creating the IcebergFullCachingLookupFunction, which will keep it consistent across task restarts or job restarts. At table creation time, CREATE TABLE dim_users (
user_id BIGINT,
name STRING,
city STRING
) WITH (
'connector' = 'iceberg',
'catalog-name' = 'iceberg_catalog',
'catalog-type' = 'hadoop',
'warehouse' = '/path/to/warehouse',
'catalog-database' = 'db',
'catalog-table' = 'users',
'lookup.full-cache.eager-load' = 'true',
'snapshot-id' = '121334'
);OR per query SELECT o.order_id, o.user_id, u.name, u.city
FROM orders AS o
LEFT JOIN iceberg_catalog.`db`.`users`
/*+ OPTIONS(
'lookup.full-cache.eager-load' = 'true',
'snapshot-id' = '121334'
) */
FOR SYSTEM_TIME AS OF o.proc_time AS u
ON o.user_id = u.user_id;There could be other strategies to simplify , instead of user specifying exact snapshot id also like latest or specific tag or as-of-timestamp |
|
@swapna267 Showing a specific snapshot ID is one approach, but it doesn't solve the problem of that snapshot expiring. On top of that, most users won't bother specifying a particular snapshot anyway. |
|
Yes it doesn't need to be particular snapshot Id. Instead it could be Latest_Snapshot or a particular Branch/Tag. I was referring to resolving it on Job Submission time instead of on TaskManagers. But i just realized you already mentioned that option. Instead of being inconsistent with lookup result across TaskManagers, prefer to have all TM's load from same Snapshot Id. And fail loudly, incase of an expired snapshot or a Tag. |
|
@swapna267 I agree that resolving the snapshot ID at Job Submission time works well when the cache is not refreshed, with an explicit failure if the snapshot has expired. However, with periodic refresh, a long-running job may encounter a failover after the original snapshot has expired. Resolving the snapshot ID at Job Submission would require manual intervention to restart the job, which doesn't seem ideal for the periodic refresh use case. I also noticed that other connectors supporting lookup joins generally establish their connections independently on each TaskManager. So I don't think resolving the snapshot ID at the Job Submission level is a good fit for this use case. This is also why I'm still leaning toward using the latest snapshot rather than pinning to a specific snapshot. |
de77ef7 to
81931fe
Compare
81931fe to
41914e4
Compare
|
@pvary @mxm @talatuyarer If you get a chance, please take a look and let me know what you think. I'd really appreciate it. |
|
Thanks @Guosmilesmile . Yes I agree, this wouldn't make sense for Periodic Refresh . As this PR's scope was limited to one time load with no periodic refresh, i was recommending that. |
| | Option | Default | Description | | ||
| | ---------------------------- |---------|-----------------------------------------------------------------------------------------------------------------------------| | ||
| | lookup.cache | | Only `FULL` is accepted; `NONE` and `PARTIAL` are rejected, because an Iceberg table cannot be point-looked-up effectively. | | ||
| | lookup.full-cache.eager-load | true | Whether to load the full cache when the lookup function is opened, instead of on the first lookup. | |
There was a problem hiding this comment.
we usually do hiearchical. So maybe lookup.cache.full.eager-load, or since we don't have full, we can just say lookup.cache.eager-load?
There was a problem hiding this comment.
I used the lookup.full-cache. prefix because I’d like the refresh-related options we add later to be consistent with the existing Flink options.
| import org.apache.flink.table.data.RowData; | ||
|
|
||
| @Internal | ||
| interface IcebergLookupCache extends Closeable { |
There was a problem hiding this comment.
Shall we introduce this only once we have more cache implementations?
There was a problem hiding this comment.
Make sense, at first I think we will add other back end in the future. Remove it now.
|
|
||
| @Override | ||
| public void add(RowData key, RowData row) { | ||
| cache.computeIfAbsent(key, k -> Lists.newLinkedList()).add(row); |
There was a problem hiding this comment.
This was my mistake. I misjudged the scale of the data here.
| RowType projectedRowType = (RowType) projected.toPhysicalRowDataType().getLogicalType(); | ||
| List<Expression> pushedFilters = filters == null ? ImmutableList.of() : filters; | ||
|
|
||
| Configuration lookupConf = Configuration.fromMap(properties); |
There was a problem hiding this comment.
Changed it to consistently use FlinkConfParser.
| lookupFunction = newLookupFunction(); | ||
| lookupFunction.open(new FunctionContext(null)); | ||
|
|
||
| Collection<RowData> rows = lookupFunction.lookup(keyRow(1L)); |
There was a problem hiding this comment.
add test with multicolumn key
| assertThat(rows).singleElement().satisfies(row -> assertRow(row, 1L, "alice", "A")); | ||
|
|
||
| // Served from the cache, not re-read from the table. | ||
| assertThat(lookupFunction.lookup(keyRow(1L))).isSameAs(rows); |
There was a problem hiding this comment.
I don't think we should rely on the cache returning the exact rows is a good idea. We might change the implementation which could return a copy of the rows, or something.
Could we just assert on the contents, or on some metrics?
There was a problem hiding this comment.
Yes, add a method assert on the contents.
5359015 to
ddef00e
Compare
|
|
||
| | Option | Default | Description | | ||
| | ---------------------------- |---------|-----------------------------------------------------------------------------------------------------------------------------| | ||
| | lookup.full-cache.eager-load | true | Whether to load the full cache when the lookup function is opened, instead of on the first lookup. | |
There was a problem hiding this comment.
nit: use `-s, and format the table
There was a problem hiding this comment.
Also we could add it to docs/docs/flink-configuration.md,?
|
|
||
| Iceberg implements lookup join with a full cache: the whole projected dimension table is loaded into the cache, and every lookup is served from it without falling back to the table. The full cache is held in memory on the TaskManager heap, so lookup join targets dimension tables that fit comfortably there. | ||
|
|
||
| The cache is loaded by default when the lookup function is opened. Set lookup.full-cache.eager-load to false to load it on the first lookup instead, which blocks the data flow until the cache is fully loaded. |
There was a problem hiding this comment.
Reword to make it clear that this is a decision between where/when to block
| * A full caching lookup function: the whole projected Iceberg dimension table is loaded into an | ||
| * in-memory cache on the first lookup, and every lookup is served from that cache. | ||
| */ | ||
| public class IcebergFullCachingLookupFunction extends LookupFunction { |
There was a problem hiding this comment.
do we need this to be public?
There was a problem hiding this comment.
Yes, we call this class in other package, I add Internal for this class now.
| private transient Counter cacheHitCounter; | ||
| private transient Counter cacheMissCounter; |
There was a problem hiding this comment.
Do we need these with only full cache supported?
There was a problem hiding this comment.
Kept only lookupMiss (probe rows whose key is not in the cached dimension table) and dropped cacheHit, which for a full cache simply equals the number of lookups.
There was a problem hiding this comment.
Do we have stat for the number of lookups?
| this.flinkConfParser = new FlinkConfParser(properties, readableConfig); | ||
| this.caseSensitive = | ||
| flinkConfParser | ||
| .booleanConf() | ||
| .option(FlinkReadOptions.CASE_SENSITIVE) | ||
| .flinkConfig(FlinkReadOptions.CASE_SENSITIVE_OPTION) | ||
| .defaultValue(FlinkReadOptions.CASE_SENSITIVE_OPTION.defaultValue()) | ||
| .parse(); |
There was a problem hiding this comment.
Maybe put into the getLookupRuntimeProvider?
|
|
||
| The cache is loaded by default when the lookup function is opened. Set lookup.full-cache.eager-load to false to load it on the first lookup instead, which blocks the data flow until the cache is fully loaded. | ||
|
|
||
| There is no background refresh: the cache keeps the data it was loaded with for the lifetime of the job, so the dimension table should be populated before the join starts. |
There was a problem hiding this comment.
Mention that the cache could be out of sync between the tasks
| options.add(FlinkCreateTableOptions.CATALOG_TABLE); | ||
| options.add(FlinkCreateTableOptions.USE_DYNAMIC_ICEBERG_SINK); | ||
| options.add(FlinkCreateTableOptions.DYNAMIC_RECORD_GENERATOR_IMPL); | ||
| options.add(LookupOptions.CACHE_TYPE); |
There was a problem hiding this comment.
Not strictly needed for behavior, but I kept it as a declaration. optionalOptions() is the only place that lists the supported options, and once the factory starts using TableFactoryHelper.validate(), an undeclared lookup.cache would fail. Happy to drop it if you'd rather.
There was a problem hiding this comment.
Remove it if we don't use it now. We can always add back later
ddef00e to
648cf75
Compare
|
|
||
| LOG.info( | ||
| "IcebergFullCachingLookupFunction loading started, snapshot={}, committedAt={}, projected fields={}, pushedFilters={}", | ||
| snapshotId == IcebergLookupReader.CURRENT_SNAPSHOT ? "none" : snapshotId, |
There was a problem hiding this comment.
shall we use null instead of a constant?
| Schema tableSchema = table.schema(); | ||
| List<String> projectedColumns = projectedRowType.getFieldNames(); | ||
| Types.NestedField[] projectedFields = new Types.NestedField[projectedColumns.size()]; | ||
| for (int i = 0; i < projectedColumns.size(); i++) { | ||
| String column = projectedColumns.get(i); | ||
| Types.NestedField field = tableSchema.findField(column); | ||
| Preconditions.checkArgument(field != null, "Cannot find column '%s' in table schema", column); | ||
| projectedFields[i] = field; | ||
| } | ||
|
|
||
| Schema icebergProjection = new Schema(projectedFields); |
There was a problem hiding this comment.
| Schema tableSchema = table.schema(); | |
| List<String> projectedColumns = projectedRowType.getFieldNames(); | |
| Types.NestedField[] projectedFields = new Types.NestedField[projectedColumns.size()]; | |
| for (int i = 0; i < projectedColumns.size(); i++) { | |
| String column = projectedColumns.get(i); | |
| Types.NestedField field = tableSchema.findField(column); | |
| Preconditions.checkArgument(field != null, "Cannot find column '%s' in table schema", column); | |
| projectedFields[i] = field; | |
| } | |
| Schema icebergProjection = new Schema(projectedFields); | |
| Schema icebergProjection = | |
| FlinkSchemaUtil.convert(table.schema(), FlinkSchemaUtil.toResolvedSchema(projectedRowType)); |
| assertThat(lookupFunction.lookup(keyRow(null, "A"))) | ||
| .singleElement() | ||
| .satisfies(row -> assertRow(row, null, "nobody", "A")); |
There was a problem hiding this comment.
Is this required from the Flink side?
There was a problem hiding this comment.
How can we reach this with SQL?
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** Tests SQL lookup join with Iceberg table source. */ | ||
| class TestIcebergLookupJoinSql extends TestSqlBase { |
There was a problem hiding this comment.
shall we add an eager load test?
| options in the DDL, or per query with the `OPTIONS` hint: | ||
|
|
||
| ```sql | ||
| SELECT o.order_id, u.name |
There was a problem hiding this comment.
SET table.dynamic-table-options.enabled=true;
There was a problem hiding this comment.
Maybe remove the example from here and just link the other page?
| The lookup options are: | ||
|
|
||
| | Option | Default | Description | | ||
| | ------------------------------ | ------- | -------------------------------------------------------------------------------------------------- | | ||
| | `lookup.full-cache.eager-load` | `true` | Whether to load the full cache when the lookup function is opened, instead of on the first lookup. | | ||
|
|
There was a problem hiding this comment.
Maybe just link the config here.
The main goal is to write everything only once
This PR adds lookup join support for the Iceberg Flink table source, using a full in-memory lookup cache.
Part of #18142
The implementation enables Iceberg tables to be used as temporal lookup join dimensions in Flink SQL.
Supported Features
Lookup join against Iceberg table source
Pushed-down filter support
Full in-memory lookup cache
lookup.cache=FULLis accepted;NONEandPARTIALare rejected, because an Iceberg table cannot be point-looked-up effectively.Configurable load strategy
lookup.full-cache.eager-loadselects when the cache is loaded:false(default): on the first lookup. Simple, but that probe row is blocked for the duration of the load.true: inopen(), so no probe row is blocked, and a load that fails fails the job at startup instead of mid-stream.Metrics
icebergLookupCachegroup:cacheHitandcacheMisscounters, andsnapshotIdandcachedRowsgauges.How to Use
Basic lookup join
Load the cache when the lookup function opens
or
Options
lookup.full-cache.eager-loadtrueopen(), instead of on the first lookup.Both can be set per join with an
OPTIONShint, or in the table DDLWITHclause.Notes